Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[blockly] Add more thing blocks #2562

Merged
merged 2 commits into from
May 7, 2024

Conversation

stefan-hoehn
Copy link
Contributor

@stefan-hoehn stefan-hoehn commented May 4, 2024

Fixes #2552.

Add missing blocks to work with a thing or a group of things

  • oh_getthing (one particular)
  • oh_getthing_state (the state of the string - to be consistent with items)
  • oh_getthings (get all things - can be used to iterate over all things)
  • oh_getthing_attribute (retrieve any attribute of a thing)
image image

Side note: I am aware that "for each item -> thing" doesn't really sound nice but it is a sad coincidence that the standard blockly for-block uses the word "item" which might be confusing. Changing this would imply a complete reimplementation of the standard block which IMHO is not worth the effort.

Signed-off-by: Stefan Höhn <mail@stefanhoehn.com>
@stefan-hoehn stefan-hoehn requested a review from a team as a code owner May 4, 2024 14:39
Copy link

relativeci bot commented May 4, 2024

#1947 Bundle Size — 10.59MiB (+0.04%).

4841133(current) vs 3fa3192 main#1945(baseline)

Warning

Bundle contains 2 duplicate packages – View duplicate packages

Bundle metrics  Change 1 change
                 Current
#1947
     Baseline
#1945
No change  Initial JS 1.86MiB 1.86MiB
No change  Initial CSS 607.87KiB 607.87KiB
Change  Cache Invalidation 18.18% 17.76%
No change  Chunks 222 222
No change  Assets 245 245
No change  Modules 2873 2873
No change  Duplicate Modules 146 146
No change  Duplicate Code 1.84% 1.84%
No change  Packages 95 95
No change  Duplicate Packages 2 2
Bundle size by type  Change 1 change Regression 1 regression
                 Current
#1947
     Baseline
#1945
Regression  JS 8.78MiB (+0.05%) 8.78MiB
No change  CSS 890.63KiB 890.63KiB
No change  Fonts 526.1KiB 526.1KiB
No change  Media 295.6KiB 295.6KiB
No change  IMG 140.74KiB 140.74KiB
No change  HTML 1.24KiB 1.24KiB
No change  Other 871B 871B

Bundle analysis reportBranch stefan-hoehn:blockly_getthingsProject dashboard

@rkoshak
Copy link

rkoshak commented May 4, 2024

Changing this would imply a complete reimplementation of the standard block which IMHO is not worth the effort.

I've noticed this too and agree. It's an unfortunate coincidence but I kind of blame OH for using the term "Item" the way it does in the first place instead of something less likely to conflict (Thing too). But since that choice was made long before even my time with OH, 🤷 .

@stefan-hoehn
Copy link
Contributor Author

re "Thing". I understand why Thing was used -> Internet of Things, so I am happy with that. Wording is always difficult, I suppose. I would rather have hoped that blockly had chosen "element" instead of item ;-)

Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me, thanks.

Just two things:

  • I would rather name the get all Things block oh_getthings instead of oh_things because this is more inline with oh_getthing.
  • Why do you have "complicated" code for the Thing attribute block? It also works with simpler code.

I will soon push my review changes for these two points, please have a look at them.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05 florian-h05 changed the title [blockly] add more thing blocks [blockly] Add more thing blocks May 7, 2024
@florian-h05 florian-h05 added enhancement New feature or request main ui Main UI labels May 7, 2024
@florian-h05 florian-h05 added this to the 4.2 milestone May 7, 2024
@florian-h05 florian-h05 merged commit 5282f44 into openhab:main May 7, 2024
6 checks passed
@florian-h05 florian-h05 deleted the blockly_getthings branch May 7, 2024 15:58
@Boldfor
Copy link

Boldfor commented Jul 11, 2024

@stefan-hoehn, thanks a lot for your work!

Two quick questions:

  1. Maybe I misunderstand something or use things wrongly (I also couldn't answer it myself with the docu):
    image
    Thr first two logging-blocks work with the new thing-loop, the third one throws an error.
2024-07-11 21:57:33.523 [ERROR] [script.javascript.thing_status_check] - Failed to execute script: TypeError: things.getThing is not a function
        at <js>.:program(<eval>:10)
        at org.graalvm.polyglot.Context.eval(Context.java:399)
        at com.oracle.truffle.js.scriptengine.GraalJSScriptEngine.eval(GraalJSScriptEngine.java:458)
        at com.oracle.truffle.js.scriptengine.GraalJSScriptEngine.eval(GraalJSScriptEngine.java:426)
        at java.scripting/javax.script.AbstractScriptEngine.eval(AbstractScriptEngine.java:262)
        ... 75 more
2024-07-11 21:57:33.524 [ERROR] [internal.handler.ScriptActionHandler] - Script execution of rule with UID 'thing_status_check' failed: org.graalvm.polyglot.PolyglotException: TypeError: things.getThing is not a function

Am I using things wrongly? If yes, maybe others run into the same problem and this usage could be blocked in blockly?

  1. With the powerful things-loop now being in place, would it make sense to be able to fire a rule if the status of any of the available things changes? The way I see the rule-trigger as of today, I can only trigger a rule if I select all things one-by-one. Is that correct?

@rkoshak
Copy link

rkoshak commented Jul 12, 2024

For 1 I think you need to file a new issue. Be sure to also post the full rule from the code tab so we can see the XML that makes up the blocks, the JS that the blocks compiled to, and the full rule context.

For 2 see https://community.openhab.org/t/thing-status-reporting-4-0-0-0-4-9-9-9/143180. Ultimately adding a new trigger would be an issue for core but this rule template can be used as is or as an example for how to work with the generic rule trigger in UI rules.

@Boldfor
Copy link

Boldfor commented Jul 13, 2024

For 1 I think you need to file a new issue. Be sure to also post the full rule from the code tab so we can see the XML that makes up the blocks, the JS that the blocks compiled to, and the full rule context.

Thanks. I just did here: #2670.
Let me know if I missed something that's needed as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
4 participants